docs(security): port security best practices 1:1 from portal#204
Merged
Conversation
Member
Author
Plan for security team reviewHi @dfinity/product-security — we'd like your input on this PR before it merges. Here's the context and what we're asking. What this PR doesThis is a 1:1 port of all security best practices from The only intentional deviations from a pure port are:
You are now added as CODEOWNER for all files in What we're asking from youPlease verify the ported content is correct and matches the portal source. We're keeping this PR in draft intentionally — it will not be merged until after a second cleanup PR (described below) is also approved. What comes next — cleanup PRAfter your approval of this PR, we'll create a second branch from
Merge sequence
|
marc0olo
force-pushed
the
docs/security-port
branch
from
51de28a to
14e63c9
Compare
This was referenced May 8, 2026
marc0olo
added a commit
that referenced
this pull request
May 12, 2026
…ces (#239) ## Summary Closes #235. Post-merge cleanup for PR #204 after PR #208 landed. - **`canister-control.md`**: SNS link → `docs/concepts/governance.md#the-service-nervous-system`; tokenomics/voting-power link → `docs/concepts/governance.md#neurons`; removed "See also" wiki bullet (no internal equivalent for SNS verification trust or swap trust content) - **`canister-upgrades.md`**: Removed wiki "current limitations" bullet for `pre_upgrade` bugs (no internal equivalent) - **`data-storage.md`**: Removed wiki "current limitations" bullet for long running upgrades and deserializer memory (no internal equivalent) - **`inter-canister-calls.md`**: Removed two wiki "current limitations" bullets for untrustworthy canisters and call graph loops (no internal equivalent) - **`data-integrity-and-authenticity.md`**: Asset certification Learn Hub link → `docs/guides/frontends/certification.md` Note: the rebase of `docs/security-port` on `main` is deferred — will be done as a final step before that PR merges. ## Sync recommendation hand-written (link fixes only; no content changes)
Replaces AI-generated security docs with verified portal content, adds 8 missing topic files, 2 new prerequisite pages, and fixes a confirmed double-spend bug in the inter-canister-calls guide. Changes: - Replace: inter-canister-calls.md, access-management.mdx, canister-upgrades.md, dos-prevention.md, data-integrity.md - Add: overview.md, data-storage.md, decentralization.md, https-outcalls.md, misc.md, observability.md, resources.md, formal-verification.md - Add: references/message-execution-properties.md (prerequisite referenced by inter-canister-calls.md) - Add: guides/canister-calls/idempotency.md (prerequisite for safe retry patterns in inter-canister calls) - Fix sidebar order conflicts (now matches portal ordering 1-14) - Fix MDX HTML comment syntax in access-management.mdx - Add security and reference diagram images to public/img/ - encryption.mdx flagged for separate security team review (new content not from portal, not changed here)
## Summary - **\"dapp\"/\"dapps\" → \"app\"/\"apps\"** across all 12 security guide files; repository names in URLs preserved (`nns-dapp`, `encrypted-notes-dapp`), link labels updated (`NNS app`, `encrypted notes`) - **\"smart contract(s)\" → \"canister(s)\"** in `decentralization.md`, including the section heading and the blockchains admonition note - **Em-dashes removed from `<!-- Upstream: -->` comments** in all 11 remaining files (`identity-and-access-management.mdx` was already fixed in a previous commit) - **Informal phrasing removed** in `data-integrity-and-authenticity.md`: \"we will club composite_query\" and \"best of both worlds\" - **Garbled sentence fixed** in `identity-and-access-management.mdx` (mobile II section): the original had a sentence fragment mid-paragraph from a copy-paste error - **\"DAO\" removed from prose** in `decentralization.md`; replaced with \"community governance\", \"governance framework\", and \"custom governance canister\" following the convention established in PR #208 - **\"decentralized governance system\" → \"governance framework\"** throughout `decentralization.md` - **`composite_query` description corrected** in `data-integrity-and-authenticity.md`: \"query call\" → \"query methods\" (composite_query is a method type, not a call type) - **\"off-chain\" → \"offchain\" / \"external\"** in `decentralization.md`; bare \"onchain\"/\"offchain\" category labels replaced with descriptive terms (\"external components\", \"hosted as canisters\") - **\"tamper-resistant\" → \"tamperproof\"** in `observability-and-monitoring.md` (one word, per brand guide) - **\"on-chain\" → \"stored in the canister\"** in `observability-and-monitoring.md` ## Sync recommendation `informed by dfinity/portal` — content is derived from the portal source but diverges intentionally for brand voice compliance; no sync back to portal is expected.
… usage (#236) ## Summary - **overview.md**: Update 3 ANSSI Rust guide links to the restructured URL paths (`introduction.html`, `unsafe/generalities.html`, `integer.html#chapter-integer`, `libraries.html#cargo-audit`) - **data-storage.md**: Remove abandoned `seniorjoinu/ic-stable-memory` library (unmaintained since May 2023) and its caution block; rephrase intro to single remaining library; update encrypted notes example link from defunct `motoko/encrypted-notes-dapp` to `rust/vetkeys/encrypted_notes_dapp_vetkd` - **decentralization.md**: Remove LaunchTrail reference (`spinner-cash/launchtrail`, abandoned June 2022); remove `basic_dao` example link (path no longer exists on master) - **canister-upgrades.md**: Update both `set_timer_interval` links from `ic-cdk/0.6.9` to `ic-cdk-timers/1.0.0` (function moved crates) - **data-integrity-and-authenticity.md**: Migrate JavaScript client-side verification example from deprecated `@dfinity/agent`, `@dfinity/candid`, `@dfinity/principal` to `@icp-sdk/core/agent`, `@icp-sdk/core/candid`, `@icp-sdk/core/principal`; update `HttpAgent`, `Certificate`, and `lookup_path` APIs to v5; fix pre-existing `start().await` bug ## Sync recommendation `sync from dfinity/portal building-apps/security/*` — changes are fixes on top of the ported content; upstream source does not yet reflect the updated SDK or fixed links.
- Replace both mermaid sequence diagrams with plantuml equivalents using the already-configured remark-plantuml plugin - Fix unity_ii_deeplink example links: main -> master branch
…ter-control (#237) ## Summary - Apply sentence case to all security guide page titles (matches portal convention and brand rules) - Add `sidebar.label: "Overview"` to overview page so navbar shows "Overview" while page title remains "Security overview" - Rename `decentralization.md` → `canister-control.md` (more accurate: covers SNS governance, canister trust verification, and untrusted asset loading) - Remove "Security" from individual page titles within the security section (the section heading already provides context, consistent with original portal structure) - Improve three descriptions: "endpoint verification" (was "validation"), "timer reinstatement after upgrades" (was "reinstatement"), added "mobile Internet Identity integration" to IAM description ## Sync recommendation hand-written (title/description metadata changes only; content unchanged)
…ces (#239) ## Summary Closes #235. Post-merge cleanup for PR #204 after PR #208 landed. - **`canister-control.md`**: SNS link → `docs/concepts/governance.md#the-service-nervous-system`; tokenomics/voting-power link → `docs/concepts/governance.md#neurons`; removed "See also" wiki bullet (no internal equivalent for SNS verification trust or swap trust content) - **`canister-upgrades.md`**: Removed wiki "current limitations" bullet for `pre_upgrade` bugs (no internal equivalent) - **`data-storage.md`**: Removed wiki "current limitations" bullet for long running upgrades and deserializer memory (no internal equivalent) - **`inter-canister-calls.md`**: Removed two wiki "current limitations" bullets for untrustworthy canisters and call graph loops (no internal equivalent) - **`data-integrity-and-authenticity.md`**: Asset certification Learn Hub link → `docs/guides/frontends/certification.md` Note: the rebase of `docs/security-port` on `main` is deferred — will be done as a final step before that PR merges. ## Sync recommendation hand-written (link fixes only; no content changes)
marc0olo
force-pushed
the
docs/security-port
branch
from
40d8bf8 to
a56e668
Compare
raymondk
previously approved these changes
May 12, 2026
raymondk
approved these changes
May 12, 2026
robin-kunzler
left a comment
robin-kunzler
left a comment
There was a problem hiding this comment.
Thanks @marc0olo ! Have a few suggestions / questions, see below.
|
|
||
| ### Recommendation | ||
|
|
||
| - Consider using an identity provider such as [Internet Identity](https://github.com/dfinity/internet-identity) for authentication, and use the ICP JavaScript agent for making canister calls. |
There was a problem hiding this comment.
maybe link to the agent here? https://github.com/dfinity/icp-js-core
Member
Author
There was a problem hiding this comment.
I guess internally linking to /references/developer-tools/#javascript--typescript makes more sense inside the docs.
Member
Author
There was a problem hiding this comment.
Member
Author
There was a problem hiding this comment.
actually I am wondering why developer tools are currently located under references as I look at this. might be worth considering to remove references from the slug.
|
|
||
| ### Security concern | ||
|
|
||
| `agent.fetchRootKey()` can be used in the ICP JavaScript agent to fetch the root subnet threshold public key from a status call in test environments. This key is used to verify threshold signatures on certified data received through canister update calls. Using this method in a production web app gives an attacker the option to supply their own public key, invalidating all authenticity guarantees of update responses. |
There was a problem hiding this comment.
same here: link to the agent? https://github.com/dfinity/icp-js-core
| @@ -0,0 +1,78 @@ | |||
| --- | |||
| title: "Properties of Message Executions on ICP" | |||
There was a problem hiding this comment.
The page is there and the direct link works, but I can't find it listed in the navigation under "References" Could you add that back?
Member
Author
There was a problem hiding this comment.
@robin-kunzler I also noticed we are cross-linking there e.g. to "Property 5". does it make sense to make the properties actual subheadings to appear in the navigation and to be able to link directly to an anchor?
|
|
||
| ### Security concern | ||
|
|
||
| ICP offers three modes of operation for canisters: `update`, `query`, and `composite_query`. For simplicity, this guide treats `composite_query` methods as query methods for the rest of this section. |
There was a problem hiding this comment.
Link to the clients calling page as reference? e.g.
Suggested change
| ICP offers three modes of operation for canisters: `update`, `query`, and `composite_query`. For simplicity, this guide treats `composite_query` methods as query methods for the rest of this section. | |
| ICP offers three modes of operation for canisters: `update`, `query`, and `composite_query`. For simplicity, this guide treats `composite_query` methods as query methods for the rest of this section. For more information, view the [detailed overview between update and query calls](https://soxyi-6iaaa-aaaam-ai2ra-cai.icp0.io/guides/canister-calls/calling-from-clients/). |
| On the other hand, query calls are fast since a single replica formulates the response, but **there is no integrity guarantee, since the response can be manipulated by a single replica or boundary node.** For example, if the NNS app fetches proposal information from the governance canister via query calls and the responding node is malicious, it can mask an ill-intentioned proposal that causes irrevocable damage as innocuous by modifying the proposal payload in the response and mislead voters into voting yes. Another consequence of query calls is that users can't rely on [canister_inspect_message](../../references/ic-interface-spec/canister-interface.md#system-api-inspect-message) as a guard. **This makes query calls, in their raw form, unfit to serve data for security-critical applications.** | ||
|
|
||
| ### Using certified variables for secure queries | ||
| In certain use cases, there is a third option whereby query results can return data that has been certified by the subnet in an earlier update call. This is the concept of certified data, and it requires changes to the update call to create the certification, the query call to return the certificate, and the frontend to verify the certificate. Using certified data provides query-like response times with update-like certified responses. |
There was a problem hiding this comment.
Bring the link back to certified variables? , e.g.
Suggested change
| In certain use cases, there is a third option whereby query results can return data that has been certified by the subnet in an earlier update call. This is the concept of certified data, and it requires changes to the update call to create the certification, the query call to return the certificate, and the frontend to verify the certificate. Using certified data provides query-like response times with update-like certified responses. | |
| In certain use cases, there is a third option whereby query results can return data that has been certified by the subnet in an earlier update call. This is the concept of certified data, and it requires changes to the update call to create the certification, the query call to return the certificate, and the frontend to verify the certificate. Using certified data provides query-like response times with update-like certified responses. This forms the core of [certified variables](https://soxyi-6iaaa-aaaam-ai2ra-cai.icp0.io/guides/backends/certified-variables/) |
|
|
||
| ### Security concern | ||
|
|
||
| The pricing of HTTPS outcalls is determined by the size of the HTTP request and the maximal response size, among other variables. Thus, if big requests are made, this could quickly drain the canister's cycles balance. This can be risky in scenarios where HTTPS outcalls are triggered by user actions (rather than a heartbeat or timer invocation). |
There was a problem hiding this comment.
Suggested change
| The pricing of HTTPS outcalls is determined by the size of the HTTP request and the maximal response size, among other variables. Thus, if big requests are made, this could quickly drain the canister's cycles balance. This can be risky in scenarios where HTTPS outcalls are triggered by user actions (rather than a heartbeat or timer invocation). | |
| The [pricing](https://soxyi-6iaaa-aaaam-ai2ra-cai.icp0.io/references/cycles-costs/#https-outcalls) of HTTPS outcalls is determined by the size of the HTTP request and the maximal response size, among other variables. Thus, if big requests are made, this could quickly drain the canister's cycles balance. This can be risky in scenarios where HTTPS outcalls are triggered by user actions (rather than a heartbeat or timer invocation). |
| order: 9 | ||
| --- | ||
|
|
||
| ## Monitor your canister |
There was a problem hiding this comment.
It seems this page is not like the original one.
The original page has two sections:
- Expose metrics from your canister
- Expose metrics from your canister
Could we bring those back?
Member
Author
There was a problem hiding this comment.
with the second section you mean Do not publicly reveal a canister's cycles balance I guess.
sure, I will apply all changes/suggestions and then come up with a PR to review.
thanks for having a detailed look!
This was referenced May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
inter-canister-calls.md)dfinity/portal(building-apps/security/) 1:1 as the content baseoverview.md,data-storage.md,decentralization.md,https-outcalls.md,miscellaneous.md,observability-and-monitoring.md,formal-verification.mdresources.mdcontent intooverview.mdas a "Further reading" section (no value as a standalone thin page)access-management.mdx→identity-and-access-management.mdxdata-integrity.md→data-integrity-and-authenticity.mdobservability.md→observability-and-monitoring.mdmisc.md→miscellaneous.mdencryption.mdx(AI-generated, unreviewed; vetKeys encryption guide will be written from scratch separately)references/message-execution-properties.mdandguides/canister-calls/idempotency.mdretry_idempotency.pngimage with a PlantUML sequence diagram inidempotency.mdmo:base/HashMapCallerGuard ininter-canister-calls.mdtomo:core/Map(only code change beyond 1:1 port)Notes
@dfinity/agentreferences in the ported files are left as-is; updating to the new JS SDK is a separate follow-upinter-canister-calls.md: it suggested issuing a refund after receiving abounded_waiterror, where the transfer could still have gone throughconcepts/security.md(new architectural overview, not from portal) is kept as-is; flagged for separate security team reviewSync recommendation
sync from dfinity/portal building-apps/security/Tracked in: #203